chore: keep eslint out of agent worktrees - #75

@claude /review

Claude finished @thecodedrift's task in 3m 5s —— View job

Review of PR #75

• Gather context (diff, changed files)
• Review .claude/hooks/worktree-create.sh
• Review .claude/hooks/worktree-remove.sh
• Review .claude/settings.json changes
• Review eslint.config.js change
• Review worktrees-pnpm SKILL.md changes
• Post overall assessment

Overall assessment

Solid, well-scoped infra change. The diff matches what the PR description claims, and the reasoning holds up under inspection:

• eslint.config.js: the new .claude/worktrees/ ignore is correctly scoped (not all of .claude/), and "lint": "eslint" in root package.json confirms this was a real problem (bare eslint walks the tree from cwd).
• Hooks (worktree-create.sh / worktree-remove.sh): both use set -euo pipefail, quote all variable expansions, and fail closed in sensible directions — creation errors propagate (non-zero exit / empty stdout fails creation, per the documented contract), while removal errs toward not deleting: uncommitted changes block removal, an unmerged branch is left alone (branch -d, not -D), and a failed git worktree remove leaves the directory for manual cleanup rather than forcing it. That asymmetry is the right call given the PR's own observation that WorktreeRemove failures aren't surfaced to the user.
• settings.json: uses the nested [{ "hooks": [...] }] form for both WorktreeCreate and WorktreeRemove, matching what the PR says is the schema-required shape (the flatter form on the docs page doesn't validate). JSON is well-formed.
• SKILL.md: cross-checked its claims against the repo — .gitignore does still list .claude/worktrees/ (line 19) as described, and the "prettier/tsc are unaffected by nesting" claim is consistent with tsc running per-package via turbo run typecheck rather than from the root. The enableGlobalVirtualStore rejection is now recorded with a concrete reason (ESM + hoisted deps) rather than left as an open question, which is a real improvement for anyone tempted to re-propose it later.

I left two inline comments on worktree-create.sh, both minor/non-blocking:

1. Line 33–37: the idempotency check only verifies the target directory exists, not that it's a real git worktree. A stray directory at that path would be returned as a success. Cheap fix would be checking git worktree list --porcelain instead of [ -d ... ].
2. Line 31: worktree_id is used unvalidated in both the directory path and the branch name. It's quoted everywhere so there's no injection risk, but an unexpected value (e.g. containing ../) could escape the intended sibling directory. Given the value originates from the trusted Claude Code harness rather than external input, this is low severity — flagged as a defense-in-depth suggestion, not a blocker.

No correctness, security, or test-adequacy issues found that would block merging. The WorktreeRemove branch-delete line (>&2 2>/dev/null) is a little cryptic (it routes success messages to stderr and swallows failure messages entirely) but does what the comment above it implies, so I didn't flag it as an issue — just noting it for whoever next has to read that line.

Re: @claude[bot] — "Claude finished @thecodedrift's task in 3m 5s — Review of PR #75…"
https://github.com/taskless/skills/actions/runs/30417133805

Both inline findings fixed in 4a70b71, and the third observation acted on too.

Idempotency check now asks git worktree list --porcelain instead of testing for a directory, so a stray path is no longer reported as a successful creation. A path that exists without being a registered worktree fails loudly rather than falling through.

worktree_id validation — constrained to ^[A-Za-z0-9][A-Za-z0-9._-]*$ with .. rejected. You rated this low severity because the value is harness-supplied, which is fair, but an implicit trust assumption tends to stop being true quietly; enforcing it costs one line. Rejection cases verified individually (../escape, -force, a..b, has space), each exiting 1 with a named reason.

The cryptic >&2 2>/dev/null you noted but did not flag: you were right that it did what the comment implied, and right that it read badly — it sent the "Deleted branch" notice to stderr and swallowed refusals entirely. Now plain >&2 || true, so both outcomes reach debug logs and neither is fatal.

— AI Coding Agent